Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(window): bind hotkey to trigger capture event #20315

Merged
merged 3 commits into from
Mar 16, 2018

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Mar 16, 2018

Relates to #20295.

This PR intends to expose additional hotkey to window to allow capture webrender. Internally it adds one new WindowEvent::CaptureWebRender for those purpose. I took some liberty to make some decisions around which need to be reviewed & updated in PR.

  • Ctrl-shift-3 is binded to hotkey to follow described in Gecko's behavior. Is it good to go?
  • Maybe do not need to create new event CaptureWebRender but reuse ToggleWebRenderDebug, having additional WebRenderDebugOption values?
    : This sounds more right path for me, but capture isn't really toggle behavior to include capture into it.
  • Capturing will create capture_webrender in cwd, creates new folder inside each time new capture stored
    : Maybe it'd better to expose new cmdline args allow overrides, or some better way else. I took the simple approach to generate path without asking for it.

  • There are tests for these changes OR
  • These changes do not require tests because _____
  • This change has manually verified on local machines (mac, windows, linux).

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @paulrouget: components/compositing/compositor.rs, components/compositing/Cargo.toml, ports/servo/glutin_app/window.rs, components/compositing/windowing.rs, components/servo/lib.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 16, 2018
@jdm
Copy link
Member

jdm commented Mar 16, 2018

This looks fantastic! Thank you for splitting the commits in a very readable way.
@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit ee637dc has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 16, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit ee637dc with merge 1ae5715...

bors-servo pushed a commit that referenced this pull request Mar 16, 2018
feat(window): bind hotkey to trigger capture event

<!-- Please describe your changes on the following line: -->
Relates to #20295.

This PR intends to expose additional hotkey to window to allow capture webrender. Internally it adds one new `WindowEvent::CaptureWebRender` for those purpose. I took some liberty to make some decisions around which need to be reviewed & updated in PR.

- `Ctrl-shift-3` is binded to hotkey to follow described in Gecko's behavior. Is it good to go?
- Maybe do not need to create new event `CaptureWebRender` but reuse `ToggleWebRenderDebug`, having additional `WebRenderDebugOption` values?
: This sounds more right path for me, but `capture` isn't really `toggle` behavior to include capture into it.
- Capturing will create `capture_webrender` in cwd, creates new folder inside each time new capture stored
: Maybe it'd better to expose new cmdline args allow overrides, or some better way else. I took the simple approach to generate path without asking for it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20295 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- This change has manually verified on local machines (mac, windows, linux).

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20315)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 1ae5715 to master...

@bors-servo bors-servo mentioned this pull request Mar 16, 2018
5 tasks
@bors-servo bors-servo merged commit ee637dc into servo:master Mar 16, 2018
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 16, 2018
@kwonoj kwonoj deleted the feat-webrender-capture branch March 16, 2018 19:22
@kvark
Copy link
Member

kvark commented Mar 16, 2018

Wonderful, thank you @kwonoj for addressing it so quickly!
FWIW, Gecko also puts a "wr.txt" file in the capture folder with the exact WR revision, which is required to do anything with the captured content. Not sure if Servo could do anything like that (reading from Cargo.lock?).

@kwonoj
Copy link
Contributor Author

kwonoj commented Mar 16, 2018

@kvark TIL, didn't know that. Let me try if I can trivially introduce it as well.

bors-servo pushed a commit that referenced this pull request Mar 23, 2018
feat(capture_webrender): write webrender revision into text

<!-- Please describe your changes on the following line: -->
Relates to #20315 (comment).

This PR try to generate `wr.txt` when trigger webrender capture. By reading gecko's implementation at [here](https://github.com/mozilla/gecko-dev/blob/3b8e63c66ae1989cfc2c7fb48ca9e025a3828e74/gfx/doc/README.webrender#L53), it seems gecko's build script generates txt file for containing revision of webrender and read it each time trigger capturing.

In this PR tries to similar in cruxwise with small differences:
- `cargo build` reads `cargo.lock`, export it into `${OUT_DIR}/`, included via macro in build time.
- when capturing triggered, those revision will be written as `wr.txt`.

Probably point of discussion & need to be updated in PR if necessary:
~- Is it acceptable `mach` generates module file on build bootstrapping? Should there be other recommendation?~ Now cargo build takes care of generation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20295 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

- This PR manually verified on local mac OS machine.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20320)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 23, 2018
…on into text (from kwonoj:feat-wr-revision); r=jdm

<!-- Please describe your changes on the following line: -->
Relates to servo/servo#20315 (comment).

This PR try to generate `wr.txt` when trigger webrender capture. By reading gecko's implementation at [here](https://github.com/mozilla/gecko-dev/blob/3b8e63c66ae1989cfc2c7fb48ca9e025a3828e74/gfx/doc/README.webrender#L53), it seems gecko's build script generates txt file for containing revision of webrender and read it each time trigger capturing.

In this PR tries to similar in cruxwise with small differences:
- `cargo build` reads `cargo.lock`, export it into `${OUT_DIR}/`, included via macro in build time.
- when capturing triggered, those revision will be written as `wr.txt`.

Probably point of discussion & need to be updated in PR if necessary:
~- Is it acceptable `mach` generates module file on build bootstrapping? Should there be other recommendation?~ Now cargo build takes care of generation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20295 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

- This PR manually verified on local mac OS machine.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 91398cf559ddeec8974e04b0a92e464669436177

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 9751ea51ab25079d0f1ac3d0f228bb97b0a61568
bors-servo pushed a commit that referenced this pull request Apr 24, 2018
fix(browser): do not omit unexpected keyevent

<!-- Please describe your changes on the following line: -->
- closes #20681

This PR intends to fix regression caused by changes I created in #20315. In code, it matches keyevent aggressively for any pattern includes `Some('3')`, ends up actual key event does not bubbles up. This PR applies correct pattern guard to pick up specific keyevent only, other events falls back to default patterns.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20681 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- manually verified
1. typing `3` in input field works
2. ctrl-shift-3 create webrender capture (verified on mac os)

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20687)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Apr 24, 2018
fix(browser): do not omit unexpected keyevent

<!-- Please describe your changes on the following line: -->
- closes #20681

This PR intends to fix regression caused by changes I created in #20315. In code, it matches keyevent aggressively for any pattern includes `Some('3')`, ends up actual key event does not bubbles up. This PR applies correct pattern guard to pick up specific keyevent only, other events falls back to default patterns.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20681 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
- manually verified
1. typing `3` in input field works
2. ctrl-shift-3 create webrender capture (verified on mac os)

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20687)
<!-- Reviewable:end -->
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 2, 2019
…on into text (from kwonoj:feat-wr-revision); r=jdm

<!-- Please describe your changes on the following line: -->
Relates to servo/servo#20315 (comment).

This PR try to generate `wr.txt` when trigger webrender capture. By reading gecko's implementation at [here](https://github.com/mozilla/gecko-dev/blob/3b8e63c66ae1989cfc2c7fb48ca9e025a3828e74/gfx/doc/README.webrender#L53), it seems gecko's build script generates txt file for containing revision of webrender and read it each time trigger capturing.

In this PR tries to similar in cruxwise with small differences:
- `cargo build` reads `cargo.lock`, export it into `${OUT_DIR}/`, included via macro in build time.
- when capturing triggered, those revision will be written as `wr.txt`.

Probably point of discussion & need to be updated in PR if necessary:
~- Is it acceptable `mach` generates module file on build bootstrapping? Should there be other recommendation?~ Now cargo build takes care of generation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20295 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

- This PR manually verified on local mac OS machine.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 91398cf559ddeec8974e04b0a92e464669436177

UltraBlame original commit: cf3b8e9c9c2a290b09376c5e2b720fe1124eeacb
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
…on into text (from kwonoj:feat-wr-revision); r=jdm

<!-- Please describe your changes on the following line: -->
Relates to servo/servo#20315 (comment).

This PR try to generate `wr.txt` when trigger webrender capture. By reading gecko's implementation at [here](https://github.com/mozilla/gecko-dev/blob/3b8e63c66ae1989cfc2c7fb48ca9e025a3828e74/gfx/doc/README.webrender#L53), it seems gecko's build script generates txt file for containing revision of webrender and read it each time trigger capturing.

In this PR tries to similar in cruxwise with small differences:
- `cargo build` reads `cargo.lock`, export it into `${OUT_DIR}/`, included via macro in build time.
- when capturing triggered, those revision will be written as `wr.txt`.

Probably point of discussion & need to be updated in PR if necessary:
~- Is it acceptable `mach` generates module file on build bootstrapping? Should there be other recommendation?~ Now cargo build takes care of generation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20295 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

- This PR manually verified on local mac OS machine.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 91398cf559ddeec8974e04b0a92e464669436177

UltraBlame original commit: cf3b8e9c9c2a290b09376c5e2b720fe1124eeacb
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 2, 2019
…on into text (from kwonoj:feat-wr-revision); r=jdm

<!-- Please describe your changes on the following line: -->
Relates to servo/servo#20315 (comment).

This PR try to generate `wr.txt` when trigger webrender capture. By reading gecko's implementation at [here](https://github.com/mozilla/gecko-dev/blob/3b8e63c66ae1989cfc2c7fb48ca9e025a3828e74/gfx/doc/README.webrender#L53), it seems gecko's build script generates txt file for containing revision of webrender and read it each time trigger capturing.

In this PR tries to similar in cruxwise with small differences:
- `cargo build` reads `cargo.lock`, export it into `${OUT_DIR}/`, included via macro in build time.
- when capturing triggered, those revision will be written as `wr.txt`.

Probably point of discussion & need to be updated in PR if necessary:
~- Is it acceptable `mach` generates module file on build bootstrapping? Should there be other recommendation?~ Now cargo build takes care of generation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #20295 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

- This PR manually verified on local mac OS machine.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 91398cf559ddeec8974e04b0a92e464669436177

UltraBlame original commit: cf3b8e9c9c2a290b09376c5e2b720fe1124eeacb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add hotkey for WebRender capturing
5 participants